-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/improve forms UI #687
Fix/improve forms UI #687
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this took so long, I keep ending up diving super deep into the code in these reviews and then don't finish them.
Nice improvements in general.
studies/forms.py
Outdated
msg = _("Je dient minstens één van de opties te selecteren") | ||
self.add_error("study_types", forms.ValidationError(msg, code="required")) | ||
msg = _("Je dient minstens één van de opties te selecteren.") | ||
self.errors["study_types"] = [msg] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind a funky solution in general. However in this case the error message doesn't come up when validating in ProposalSubmit.
This could have something to do with the fact that this ModelForm doesn't have any "official" fields, as in fields that directly relate to the model. Or more specifically that the error added doesn't relate to a model field.
That's just a hunch though, might be to do with the BootstrapCheckboxMultiple or the error code not being reached.
I'd feel better with going with the funky solution if you could look into that in this PR. Feel free to give me a call if you get stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this file, SessionOverviewForm should not be in the following list:
if issubclass(
form_class,
(
InterventionForm,
ObservationForm,
SessionUpdateForm,
SessionOverviewForm,
),
):
kwargs["study"] = obj.study
As it has Study as its object already, so obj.study
doesn't exist and throws an exception.
Not on-topic for this PR, feel free to convert this to a separate issue.
tasks/templates/tasks/task_list.html
Outdated
@@ -48,7 +48,7 @@ | |||
</table> | |||
{% else %} | |||
<div class="alert alert-info" role="alert"> | |||
{% trans "Deze sessie bevat nog geen taken. Klik op het podloodje om taken aan te maken!" %} | |||
{% trans "Deze sessie bevat nog geen taken. Tijd om er één aan te maken!" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translation hasn't been updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we create a seperate issue for translation ... Because a lot of stuff is not translated or not up to date ...
@@ -296,7 +296,7 @@ def get_form_errors(proposal: Proposal) -> list: | |||
instance = form_class(**kwargs) | |||
|
|||
for field, error in instance.errors.items(): | |||
if field in instance.fields: | |||
if field in instance.fields or field == "__all__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, can't believe we weren't doing this yet.
Ok, So I've adressed your comments, and I also found some bugs, which I fixed along the way. I noticed that StudyEndForm was missing from the validation, so I've added that. (I see now that I put the wrong commit message regarding this ... oh well.) -> 9899ca7 I found a bug in this form. It was, unnecessarily receiving a study kwarg, this is now fixed. And I tried to work on StudyDesignForm, and made some changes that will hopefully make this form easier to validate externally. However, as I showed, I did run into a really strang bug, where this form does not get properly initialized in the validator. Its clean method does not get called, and I cannot figure out why this is. I would really appreciate if you could have a look at this, because it is driving me crazy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, assuming that the study kwarg comment gets fixed. The orthography issue is of course minor
studies/forms.py
Outdated
# error msg to a built-in required error message. | ||
if not "study_types" in cleaned_data: | ||
error = forms.ValidationError( | ||
_("Je dient minstens één van de opties te selecteren."), code="required" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recently learned (guess from who) that you're not supposed to write "één" in Dutch if there's no chance of ambiguity. Same with "Tijd om er één aan te maken!", by the book it shouldn't have accents.
@@ -285,7 +304,6 @@ def __init__(self, *args, **kwargs): | |||
- Remove empty label from deception/negativity/stressful/risk field and reset the choices | |||
- mark_safe the labels of negativity/stressful/risk | |||
""" | |||
self.study = kwargs.pop("study", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This View is still getting a study
kwarg argument from somewhere, causing it to error out.
Here is a mishmash of different small issues. Mostly to do with improving error displays, and general UI improvements for the forms, mostly around sessions and tasks ...
I also found out the non-field-errors were not caught by validation, so I added a small fix for this.